Expose adaptive context APIs for Swift LLM#512
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds adaptive-context APIs across the C, C++, and Swift layers, with new component wrappers, lifecycle/proto entry points, symbol exports, bridge methods, and public RunAnywhere helpers. ChangesLLM Adaptive Context API
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Related issue: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 369d02c. Configure here.
|
Could a maintainer please approve the GitHub Actions run for this fork PR when you get a chance? I’m contributing from a Windows environment, so I can’t run the macOS/iOS Swift checks locally. I’ve included the local static checks I was able to run in the PR description. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge`+LLM.swift:
- Around line 145-152: The error handling in throwIfFailed currently converts
all non-success statuses into a generic SDKException and drops the native
backend detail. Update CppBridge+LLM.throwIfFailed to include
rac_error_message(status) in the thrown message, especially when status is
RAC_ERROR_NOT_SUPPORTED or RAC_ERROR_FEATURE_NOT_AVAILABLE, so callers can
distinguish unsupported-backend failures from other processing errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 357d08be-b462-415a-9b02-74bd7babe36e
📒 Files selected for processing (8)
sdk/runanywhere-commons/exports/RACommons.exportssdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_service.hsdk/runanywhere-commons/src/features/llm/llm_module.cppsdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_component.hsdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_service.hsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/LLM/RunAnywhere+TextGeneration.swift
|
Validation update after addressing the review feedback:
The only failed lane was |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/runanywhere-commons/src/features/llm/llm_module.cpp (1)
2153-2194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the near-identical lifecycle wrapper boilerplate.
rac_llm_inject_system_prompt_lifecycle,rac_llm_append_context_lifecycle, andrac_llm_clear_context_lifecycleall repeat the same shape: validate →#if !RAC_HAVE_PROTOBUFguard →acquire_lifecycle_llm→ ternary dispatch on a vtable op →release_lifecycle_llm→ return. A small helper (e.g., templated on the vtable member pointer, or a lambda-based wrapper) would collapse these into a few lines and reduce future copy/paste risk when more adaptive-context ops are added.As per coding guidelines, "Focus on SIMPLICITY and follow Clean SOLID principles when writing code with emphasis on reusability, clean architecture style, and clear separation of concerns" for
sdk/runanywhere-commons/**/*.{cpp,h,hpp,c}.♻️ Illustrative consolidation sketch
+namespace { +template <typename Op, typename... Args> +rac_result_t call_lifecycle_op(Op op, Args&&... args) { +#if !defined(RAC_HAVE_PROTOBUF) + return RAC_ERROR_FEATURE_NOT_AVAILABLE; +#else + rac::llm::LifecycleLlmRef ref; + rac_result_t rc = rac::llm::acquire_lifecycle_llm(&ref); + if (rc != RAC_SUCCESS) return rc; + rc = (ref.ops && (ref.ops->*op)) ? (ref.ops->*op)(ref.impl, std::forward<Args>(args)...) + : RAC_ERROR_NOT_SUPPORTED; + rac::llm::release_lifecycle_llm(&ref); + return rc; +#endif +} +} // namespace + rac_result_t rac_llm_inject_system_prompt_lifecycle(const char* prompt) { - if (!prompt) { - return RAC_ERROR_INVALID_ARGUMENT; - } -#if !defined(RAC_HAVE_PROTOBUF) - return RAC_ERROR_FEATURE_NOT_AVAILABLE; -#else - rac::llm::LifecycleLlmRef ref; - rac_result_t rc = rac::llm::acquire_lifecycle_llm(&ref); - if (rc != RAC_SUCCESS) { - return rc; - } - - rc = (ref.ops && ref.ops->inject_system_prompt) - ? ref.ops->inject_system_prompt(ref.impl, prompt) - : RAC_ERROR_NOT_SUPPORTED; - rac::llm::release_lifecycle_llm(&ref); - return rc; -#endif + if (!prompt) return RAC_ERROR_INVALID_ARGUMENT; + return call_lifecycle_op(&rac_llm_ops_t::inject_system_prompt, prompt); }Also applies to: 2282-2298
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/runanywhere-commons/src/features/llm/llm_module.cpp` around lines 2153 - 2194, The lifecycle adaptive-context wrappers are duplicating the same validate/acquire/dispatch/release pattern across rac_llm_inject_system_prompt_lifecycle, rac_llm_append_context_lifecycle, and rac_llm_clear_context_lifecycle. Refactor this into a small shared helper inside llm_module.cpp, ideally one that takes the relevant vtable op (or a lambda) and handles the RAC_HAVE_PROTOBUF guard, rac::llm::acquire_lifecycle_llm, null-op fallback to RAC_ERROR_NOT_SUPPORTED, and rac::llm::release_lifecycle_llm consistently. Keep the individual public wrappers as thin calls into that helper so future adaptive-context lifecycle ops can be added without copy/paste.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdk/runanywhere-commons/src/features/llm/llm_module.cpp`:
- Around line 2153-2194: The lifecycle adaptive-context wrappers are duplicating
the same validate/acquire/dispatch/release pattern across
rac_llm_inject_system_prompt_lifecycle, rac_llm_append_context_lifecycle, and
rac_llm_clear_context_lifecycle. Refactor this into a small shared helper inside
llm_module.cpp, ideally one that takes the relevant vtable op (or a lambda) and
handles the RAC_HAVE_PROTOBUF guard, rac::llm::acquire_lifecycle_llm, null-op
fallback to RAC_ERROR_NOT_SUPPORTED, and rac::llm::release_lifecycle_llm
consistently. Keep the individual public wrappers as thin calls into that helper
so future adaptive-context lifecycle ops can be added without copy/paste.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bac05259-0d09-4190-97f8-fdea89942a8a
📒 Files selected for processing (8)
sdk/runanywhere-commons/exports/RACommons.exportssdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_service.hsdk/runanywhere-commons/src/features/llm/llm_module.cppsdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_component.hsdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_service.hsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/LLM/RunAnywhere+TextGeneration.swift
|
@coderabbitai The full review still reports 52.17% docstring coverage after commit ad51147 documented the eight new C++ adaptive-context definitions and the Swift native-status helper. Please identify the exact functions you still count as undocumented so we can address only concrete gaps. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RunanywhereAI/runanywhere-sdks"
PR=512
# Get PR head ref info
gh pr view $PR --repo $REPO --json headRefName,headRepositoryOwner,headRepository -q '.'
# Fetch the branch to inspect files
gh pr diff $PR --repo $REPO > /tmp/pr512.diff
wc -l /tmp/pr512.diffLength of output: 699 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check for doc comments preceding the new functions in the diff
grep -n "rac_llm_component_inject_system_prompt\|rac_llm_component_append_context\|rac_llm_component_generate_from_context\|rac_llm_component_clear_context\|rac_llm_inject_system_prompt_lifecycle\|rac_llm_append_context_lifecycle\|rac_llm_generate_from_context_proto\|rac_llm_clear_context_lifecycle\|injectSystemPrompt\|appendContext\|generateFromContext\|clearContext\|throwIfFailed" -B5 /tmp/pr512.diff | head -400Length of output: 869 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="RunanywhereAI/runanywhere-sdks"
PR=512
gh pr diff $PR --repo $REPO > /tmp/pr512.diff
wc -l /tmp/pr512.diff
echo "---- llm_module.cpp new functions context ----"
grep -n "^+.*rac_llm_component_inject_system_prompt\|^+.*rac_llm_component_append_context\|^+.*rac_llm_component_generate_from_context\|^+.*rac_llm_component_clear_context\|^+.*rac_llm_inject_system_prompt_lifecycle\|^+.*rac_llm_append_context_lifecycle\|^+.*rac_llm_generate_from_context_proto\|^+.*rac_llm_clear_context_lifecycle" /tmp/pr512.diff
echo "---- Swift new methods context ----"
grep -n "^+.*func injectSystemPrompt\|^+.*func appendContext\|^+.*func generateFromContext\|^+.*func clearContext\|^+.*func throwIfFailed" /tmp/pr512.diffLength of output: 4206 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect doc comments directly above each function definition in the diff
sed -n '160,260p' /tmp/pr512.diff
echo "======="
sed -n '380,430p' /tmp/pr512.diff
echo "======="
sed -n '555,650p' /tmp/pr512.diffLength of output: 10603 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '260,310p' /tmp/pr512.diff
echo "===RunAnywhere+TextGeneration rest==="
sed -n '640,690p' /tmp/pr512.diffLength of output: 4058 All eight new C++ adaptive-context definitions and the four Swift native-status/public methods I can see in this diff do carry doc comments ( A few things worth verifying on your end:
If you can point me to the exact CodeRabbit review comment/report containing that 52.17% figure (or its comment ID/permalink), I can cross-reference it directly against the current diff and give you a concrete list of symbols it's still counting as undocumented, rather than guessing at the source of the metric. ✅ Action performedReview finished.
|
shubhamsinnh
force-pushed
the
issue-500-adaptive-context
branch
from
ad51147 to
2ba875f
Compare
|
Hi @shubhammalhotra28 , this one's ready for review. All CI checks passing, feedback addressed. |
|
Hey bro @shubhamsinnh |
Hi @Siddhesh2377 thanks for the nudge, getting this checked. |
@Siddhesh2377 merge conflict resolved, it should be clean now. |
|
Hi @shubhammalhotra28 @Siddhesh2377 — following up here. The earlier feedback/conflict should be resolved now. Please let me know if you want any changes from my side. |
# Conflicts: # sdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_component.h # sdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_service.h
# Conflicts: # sdk/runanywhere-commons/src/features/llm/llm_module.cpp
shubhamsinnh
force-pushed
the
issue-500-adaptive-context
branch
from
632dda5 to
e4535bc
Compare
2 participants
Description
Exposes adaptive-context LLM operations through the component and lifecycle-owned C ABI, then adds matching Swift public APIs so callers can reuse the model loaded through the existing
RunAnywhere.loadModelflow.Changes include:
RunAnywhere.injectSystemPrompt,appendContext,generateFromContext, andclearContextThe public Swift path remains lifecycle-owned, avoiding a second model/service handle and its associated memory cost.
Fixes #500
Type of Change
Testing
Validation performed:
git diff --checkswift-spm: passedios-device: passedmacos-debug: passedmacos-releasefailed before this PR's code while compiling committed generated protobuf headers against Homebrew protobuf 35.1;macos-debugpassed in the same matrixmain:mainPlatform-Specific Testing (check all that apply)
Swift SDK / iOS Sample:
Kotlin SDK / Android Sample:
Flutter SDK / Flutter Sample:
React Native SDK / React Native Sample:
Playground:
Web SDK / Web Sample:
Labels
SDKs:
Swift SDKKotlin SDKFlutter SDKReact Native SDKWeb SDKCommonsSample Apps:
iOS SampleAndroid SampleFlutter SampleReact Native SampleWeb SampleChecklist
Screenshots
Not applicable; this PR adds SDK APIs without changing UI.
Summary by CodeRabbit
injectSystemPrompt,appendContext,generateFromContext,clearContext).